Skip to content

refactor(backend): centralize per-band Butterworth filtering in BatchedBandpass - #44

Merged
slittycode merged 3 commits into
mainfrom
claude/review-torchfx-inspiration-LpSZq
May 14, 2026
Merged

refactor(backend): centralize per-band Butterworth filtering in BatchedBandpass#44
slittycode merged 3 commits into
mainfrom
claude/review-torchfx-inspiration-LpSZq

Conversation

@slittycode

Copy link
Copy Markdown
Owner

Context

The torchfx audit (docs/history/library-review-torchfx-2026-05-13.md, PR #39) ruled out adopting matteospanio/torchfx as a dependency — GPL v3 vs ASA's MIT is a one-way door — but flagged the underlying pattern as worth borrowing: a small filter primitive that batches per-band work and gives a clean hook for future GPU acceleration. This PR implements the minimum slice of that pattern that earns its complexity today: PR 1 of the audit's plan.

What changed

Three scipy-Butterworth bandpass call sites in analyze_detection.py (_bandpass_signal, the reverb per-band RT60 loop) were each re-implementing the same butter(4, [lo, hi], output='sos') + sosfiltfilt sequence with slightly different glue. They now share one BatchedBandpass instance per sample rate (cached via functools.lru_cache(maxsize=4)) that memoises SOS coefficients per (lo_hz, hi_hz).

The 7-band transient-density loop in analyze_per_band_transient_density and the Essentia BandPass call sites are intentionally out of scope for this PR. The transient-density swap is a candidate for a follow-on PR if and only if this one lands cleanly.

Files

File Change
apps/backend/dsp_bandbank.py New — BatchedBandpass class (~120 LOC incl. docstrings)
apps/backend/analyze_detection.py _bandpass_signal shrunk to one-line delegation; reverb loop dispatches via bandbank.filter_many(_REVERB_BANDS, dtype=np.float32)
apps/backend/tests/test_dsp_utils.py New BatchedBandpassTests class (8 tests)

Bit-identicality

Output is bit-identical to the previous inline implementation. The load-bearing test test_filter_one_matches_inline_scipy_bit_for_bit compares the new class against a literal copy of the pre-refactor _bandpass_signal arithmetic on a fixed-seed signal and asserts np.allclose(atol=1e-12, rtol=1e-12). The full pre/post snapshot of analyze.py output on a synthetic click-train fixture is zero-diff — including reverbDetail.perBandRt60. Phase 1 numbers do not move.

No JSON field changes, no schema changes, no new dependencies.

Forward-looking seam

BatchedBandpass(backend=...) is reserved for a future torch path — only "scipy" is implemented today; passing anything else raises ValueError. A future torch backend would land here if profiling shows per-band scipy filtering is hot. Until then, this is the simplest shape that earns its keep on consolidation alone.

Tests

$ ./venv/bin/python -m unittest tests.test_dsp_utils.BatchedBandpassTests -v
test_filter_many_accepts_reverb_bands_tuple_shape ... ok
test_filter_many_accepts_spectral_balance_dict_shape ... ok
test_filter_many_returns_same_arrays_as_per_band_loop ... ok
test_filter_one_matches_inline_scipy_bit_for_bit ... ok
test_filter_one_returns_none_for_empty_input ... ok
test_invalid_band_returns_none_or_absent_key ... ok
test_sos_coefficients_are_cached_per_band ... ok
test_unsupported_backend_raises ... ok
Ran 8 tests in 0.145s — OK

$ ./venv/bin/python -m unittest tests.test_analyze.ReverbDetailTests
Ran 7 tests in 1.796s — OK

$ ./venv/bin/python -m unittest discover -s tests
Ran 551 tests in 138.571s — OK

End-to-end pre/post snapshot of analyze.py on /tmp/reverb_test.wav (synthetic click train + 80 Hz drone): diff produces zero output.

Why this is worth it

This is a maintenance change per PURPOSE.md decision framework category #4 — it doesn't move Phase 1 numbers, doesn't change Phase 2 recommendations, isn't on the critical latency path. What it does buy: one place to add a torch-backed backend="torch" variant if profiling ever shows per-band scipy filtering is a hotspot, plus the audit conclusion ("don't add the dep, borrow the pattern") now has a concrete shape in the tree.

https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk


Generated by Claude Code

claude added 2 commits May 13, 2026 21:54
Library evaluation of matteospanio/torchfx against ASA's per-band
filtering call sites. Conclusion: do not adopt — GPL v3 vs ASA's
MIT license is a one-way door, the shipped filter inventory is too
sparse to displace scipy, and Phase 1 bandpass cost is not on the
critical path. The torchfx |/+ operator pattern and the (tensor, fs,
device) carrier object are worth borrowing only if a future profile
shows per-band filtering is a top-three Phase 1 cost on long tracks.

https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk
…edBandpass

The torchfx audit (docs/history/library-review-torchfx-2026-05-13.md, PR #39)
ruled out adopting matteospanio/torchfx as a dependency (GPL v3 vs ASA's MIT)
but flagged the underlying pattern — a small filter primitive that batches
per-band work and gives a clean hook for future GPU acceleration — as worth
borrowing internally. This commit implements the minimum slice of that
pattern that earns its complexity today: PR 1 of the audit's plan.

Three scipy-Butterworth bandpass call sites in analyze_detection.py
(_bandpass_signal, the reverb per-band RT60 loop) were each re-implementing
the same butter(4, [lo, hi], output='sos') + sosfiltfilt sequence with
slightly different glue. They now share one BatchedBandpass instance per
sample rate (cached via functools.lru_cache(maxsize=4)) that memoises SOS
coefficients per (lo_hz, hi_hz). The 7-band transient-density loop in
analyze_per_band_transient_density and the Essentia BandPass call sites
are intentionally out of scope for this PR.

Output is bit-identical to the previous inline implementation. The
load-bearing test (test_filter_one_matches_inline_scipy_bit_for_bit) compares
the new class against a literal copy of the pre-refactor _bandpass_signal
arithmetic on a fixed-seed signal and asserts np.allclose(atol=1e-12,
rtol=1e-12). The full pre/post snapshot of analyze.py output on a synthetic
click-train fixture is zero-diff (including reverbDetail.perBandRt60), so
Phase 1 numbers do not move.

No JSON field changes, no schema changes, no new dependencies. The
BatchedBandpass(backend=...) argument is a forward-looking seam — only
"scipy" is implemented today; a future torch path would land here if
profiling shows per-band scipy filtering is hot.

Tests:
  - tests.test_dsp_utils.BatchedBandpassTests: 8/8 OK (incl. bit-identical
    parity for both filter_one and filter_many)
  - tests.test_analyze.ReverbDetailTests: 7/7 OK (unchanged)
  - discover -s tests: 551/551 OK

https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk

@slittycode slittycode left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE

Summary

Pure internal refactor of analyze_detection.py's Butterworth bandpass call sites into a new BatchedBandpass utility class. Phase 1 output is bit-identical by construction and verified by a literal copy-paste of the pre-refactor arithmetic as a reference oracle. All 551 backend tests pass per the PR description. No schema changes, no new external dependencies, no JSON field changes.

Findings

Worth considering — filter_one / filter_many output dtype asymmetry (dsp_bandbank.py:67–91)

filter_one hardcodes float32 (it has to — bit-identical parity with _bandpass_signal). filter_many defaults to float64. The two public methods on the same class silently return different dtypes with no-arg calls, which is a footgun for the analyze_per_band_transient_density migration the PR is explicitly anticipating. The current call site (filter_many(..., dtype=np.float32)) is fine, so this is not a regression. But the intended follow-on PR will need to call filter_many with no dtype (float64 result) and then cast only for the librosa call — which is the opposite of what filter_one does. Consider either adding a dtype parameter to filter_one for consistency, or documenting the intentional asymmetry more prominently than a test comment.

This is a judgment call and not a blocker.

Test results

551 / 551 pass (per PR description with command output). BatchedBandpassTests: 8/8, ReverbDetailTests: 7/7. The load-bearing test (test_filter_one_matches_inline_scipy_bit_for_bit) asserts np.allclose(atol=1e-12, rtol=1e-12) against a literal pre-refactor copy — correct approach and the right threshold (well below float32 ULP at this magnitude).

Phase boundary check

Clean. Only analyze_detection.py (Phase 1 internal) and the new dsp_bandbank.py (Phase 1 utility) are modified. No Phase 2/3 files touched. analyze.py stdout contract and JSON_SCHEMA.md are unchanged.


Generated by Claude Code

Addresses review feedback on PR #44: the original filter_one hardcoded
float32 while filter_many defaulted to float64, which would be a footgun
for the planned analyze_per_band_transient_density migration (PR 2 of
the audit). filter_one now accepts a keyword-only ``dtype`` argument,
making the public API symmetric with filter_many.

The default remains ``np.float32`` — bit-identicality with the pre-refactor
_bandpass_signal is preserved, and the existing reverb call site is
unchanged. The asymmetric defaults (float32 for filter_one, float64 for
filter_many) are intentional and now documented in a class-level note:
each default matches its primary use case verbatim, and callers can
override either at the call site.

Two new tests lock the contract:
  - test_filter_one_default_dtype_is_float32 — asserts default is float32
    (bit-identicality guarantee with _bandpass_signal)
  - test_filter_one_respects_dtype_override — asserts dtype=np.float64
    produces a float64 array (the forward-looking override path)

BatchedBandpassTests: 10/10 OK. ReverbDetailTests: 7/7 OK unchanged.

https://claude.ai/code/session_01WJ4q6kzeEMscgnSa9gRqQk
@slittycode
slittycode merged commit 0fff7ba into main May 14, 2026
2 checks passed
@slittycode
slittycode deleted the claude/review-torchfx-inspiration-LpSZq branch May 14, 2026 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants